Skip to content

Conversation

@alexshoe
Copy link
Contributor

@alexshoe alexshoe commented Dec 4, 2025

Description:

Extends xref and yref to accept arrays, allowing shapes to span multiple subplots with each vertex anchored to a different axis. See #7151 for more information.

Example:

shapes: [{  
  type: 'rect',  
  x0: 0, 
  x1: 1, 
  y0: 0, 
  y1: 1,  
  xref: ['x', 'x2'],  // x0 uses 'x', x1 uses 'x2'  
  yref: ['y', 'y2']   // y0 uses 'y', y1 uses 'y2'
}]

Progress:

  • Step 1: Attributes & validation
    • Extend xref and yref to allow array values
    • Add helper function to check number of defining coordinates of a shape
    • Update value validation to work with array values
  • Step 2: Refactor coordinate conversion
  • Step 3: Update shape rendering
  • Step 4: Addressing editable shapes
  • Step 5: Tests + documentation

@alexshoe alexshoe marked this pull request as draft December 4, 2025 22:16
@alexshoe alexshoe marked this pull request as ready for review January 1, 2026 19:17
@alexshoe alexshoe requested a review from emilykl January 1, 2026 19:17
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexshoe Could you add a label to each of these shapes to make sure everything works as expected?

@emilykl
Copy link
Contributor

emilykl commented Jan 23, 2026

@alexshoe This is looking really good. 🎉

I left some minor comments on the implementation (mostly style and readability) and also noticed some issues with the Jasmine tests. Could you do a pass through all the Jasmine tests and make sure they make sense, are testing the right things, etc.

I also realized there's a pretty good chance that shape.label.texttemplate won't play nice with this feature at all. I would expect variables x0, x1, y0, y1 to work as expected, but the remaining ones simply don't make any sense when the endpoints are associated with different axes. Can you test out what happens? There should be a decent error case (ignore the variable, perhaps?) rather than a catastrophic failure or showing the wrong information.

@alexshoe
Copy link
Contributor Author

@emilykl Thanks for the comments! I tested texttemplate with multi-axis shapes and it doesn't cause an error but the derived values (e.g. width, height, slope, et.c) don't make any sense as you predicted. Raw values (x0, x1, y0, y1) work fine but something like width would be meaningless since we're mixing coordinates from different axes. Should I document this limitation somewhere or maybe throw an error?

Meanwhile, I also modified the code to display labels for non-path shapes since it previously didn't work well with multi-axis references. Also added labels to the image test to confirm that it works.

@emilykl
Copy link
Contributor

emilykl commented Jan 26, 2026

@alexshoe Thanks to @camdecoster 's work in #7577 , we now have the shape.texttemplatefallback attribute, which is already used in cases where the given variable name is invalid. So I propose we use texttemplatefallback for the derived variables for multi-axis shapes. See the text_on_shapes_texttemplate mock for an example of how this works in practice (%{length} on the rectangle in the upper-right subplot is replaced with - because length is an invalid variable for rectangles).

Good news that raw values work as expected!

beforeEach(function() { gd = createGraphDiv(); });
afterEach(destroyGraphDiv);

function getShape(index) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
function getShape(index) {
function getShapesWithIndex(index) {

expect(lineBBox.left).toBeCloseTo(lineExpectedLeft);
expect(lineBBox.right).toBeCloseTo(lineExpectedRight);
var lineBBox = getBoundingBox(0);
expect(lineBBox.left).toBeCloseTo(xa.l2p(1) + xa._offset);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only issue with this testing approach is the question of whether xa._offset itself might ever have an incorrect value due to the presence of a multi-axis shape. Do you know where/how xa._offset is calculated?

If you think that's not possible, this is OK.

expect(gd._fullLayout.xaxis2.range[0]).toBeLessThan(1);
expect(gd._fullLayout.xaxis2.range[1]).toBeGreaterThan(5);
expect(gd._fullLayout.yaxis2.range[0]).toBeLessThan(1);
expect(gd._fullLayout.yaxis2.range[1]).toBeGreaterThan(5);
Copy link
Contributor

@emilykl emilykl Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably use .toBeCloseTo() here as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Shapes that reference multiple axes simultaneously

4 participants